send old version memoryQueue's stale activation to queueManager when update action #5228

ningyougang · 2022-04-28T06:33:17Z

Send old version memoryQueue's stale activation to queueManager when update action if doesn't exist old version container.

This is bug, if doesn't fix, the activation will be failed as below

activation processing is not initiated for 60000 ms

Add exponential delay for container creation retry

This is enhance point, during retry,

wait 100.milliseconds for 0th retry
wait 200.milliseconds for 1th retry
wait 400.milliseconds for 2th retry
wait 800.milliseconds for 3th retry
wait 1600.milliseconds for 4th retry

change scheduler's scheduler_inProgressJobRetentionSecond to scheduler_inProgressJobRetention

Description

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

When old container doesn't exist

ningyougang · 2022-04-28T06:37:03Z

...eduler/src/main/scala/org/apache/openwhisk/core/scheduler/container/CreationJobManager.scala

@@ -97,7 +96,7 @@ class CreationJobManager(feedFactory: (ActorRefFactory, String, String, Int, Arr
        if (retryCount >= retryLimit || !error.exists(ContainerCreationError.whiskErrors.contains)) {
          logging.error(
            this,
-            s"[$creationId] Failed to create container $retryCount/$retryLimit times for $cause. Finished creation")
+            s"[$action] [$creationId] Failed to create container $retryCount/$retryLimit times for $cause. Finished creation")


Make it easy to debug in future maybe.

codecov-commenter · 2022-04-28T09:28:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.98%. Comparing base (3e3414c) to head (f7f8fff).
Report is 157 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5228       +/-   ##
===========================================
+ Coverage   44.53%   72.98%   +28.44%     
===========================================
  Files         238      238               
  Lines       13957    13965        +8     
  Branches      570      576        +6     
===========================================
+ Hits         6216    10192     +3976     
+ Misses       7741     3773     -3968

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

style95 · 2022-04-29T09:30:06Z

core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala

@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
      // let QueueManager know this queue is no longer in charge.
      context.parent ! staleQueueRemovedMsg

+      if (queue.size > 0) {
+        // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+        if (containers.size == 0) {


I think we need to consider containers being created(in-progress containers).
And it's worth discussing but IIRC, in our downstream, we decided to promote the old version of activations to the latest one rather than just completing them with errors.
If we do this, I think QueueManager should act in the same way for stale activations.

Regarding I think we need to consider containers being created(in-progress containers).
I think have no need to consider in-progress containers during action update in this case, because

Normally, the in-progress containers will be failed due to version not matched: https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/ContainerMessageConsumer.scala#L85

if above #L85 codes happens, has a option to create container using latest version of action, but if we do this, the new version container can't fetch the old version memoryQueue's stale activation as well.

So here, i think just consider if (containers.size == 0) is enough.

Regarding

And it's worth discussing but IIRC, in our downstream, we decided to promote the old version of activations to the latest one rather than just completing them with errors. If we do this, I think QueueManager should act in the same way for stale activations.

when this case happens and have no existing old version container, just send the stale activation to queueManager to schedule to new version memoryQueue, and the activations will be fetched by new version container.

ok, so this is to promote the activation revision by sending them to QueueManager.

jiangpengcheng · 2022-05-07T06:38:10Z

tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala

          probe.expectMsg(Transition(fsm, state, Removing))

          fsm ! QueueRemovedCompleted

-          // queue should not be terminated as there is an activation


why these lines are removed? since there is a container in https://github.com/apache/openwhisk/pull/5228/files#diff-4b5788222d7949baf4e09b570e4fa49d3fa79e5432ff50c798b04353758c8d4aR1539, I think message should not be sent to QueueManager?

jiangpengcheng · 2022-05-07T06:42:02Z

tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala

@@ -1496,6 +1497,7 @@ class MemoryQueueFlowTests
          // queue should not be terminated as there is an activation
          Thread.sleep(gracefulShutdownTimeout.toMillis)

+          fsm.underlyingActor.containers = Set(testContainerId)


adding this line covers case that there is still a container for old queue, but does not cover case that there is no container for old queue

Updated accordingly.

ningyougang · 2022-05-07T09:16:15Z

tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueTests.scala

-    memoryQueue.creationIds.size shouldBe 1
+    // the monit actor in memoryQueue may decide to create a container
+    memoryQueue.creationIds.size should be >= 1
+    memoryQueue.creationIds.size should be <= 2


Due to this codes: https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala#L908
Above #L908 code is good due to store the inprogressId to etcd needs some time, so when containerCreationMessage created, need to add that creationId to creationIds in advance.

This test case may not stable, e.g. due to the creationId(testId1) doesn't keep consistent with CreationId.generate(), if the monit actor decide to create a container very quickly, this test case would be failed.

So here, make it stable.

ningyougang · 2022-05-07T09:20:20Z

tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/QueueManagerTests.scala

@@ -554,7 +554,7 @@ class QueueManagerTests
          mockConsumer,
          QueueManagerConfig(maxRetriesToGetQueue = 2, maxSchedulingTime = 10 seconds)))

-    queueManager ! activationMessage
+    queueManager ! activationMessage.copy(transid = TransactionId(messageTransId.meta.copy(start = Instant.now())))


Make this test case more stable

style95 · 2022-05-09T01:38:44Z

core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala

@@ -829,6 +834,23 @@ class MemoryQueue(private val etcdClient: EtcdClient,
    }
  }

+
+  private def handleStaleActivationsWhenUpdateAction(queueManager: ActorRef): Unit = {


Suggested change

private def handleStaleActivationsWhenUpdateAction(queueManager: ActorRef): Unit = {

private def handleStaleActivationsWhenActionUpdated(queueManager: ActorRef): Unit = {

Updated the method name accordingly.

style95 · 2022-05-09T01:42:37Z

core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala

@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
      // let QueueManager know this queue is no longer in charge.
      context.parent ! staleQueueRemovedMsg

+      if (queue.size > 0) {
+        // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+        if (containers.size == 0) {


ok, so this is to promote the activation revision by sending them to QueueManager.

ningyougang · 2022-05-09T05:52:09Z

I will merge this pr due to this can make some test cases more stable

Complele old activation when update action

d521906

When old container doesn't exist

ningyougang commented Apr 28, 2022

View reviewed changes

style95 reviewed Apr 29, 2022

View reviewed changes

ningyougang added 3 commits May 5, 2022 14:39

Send old version activation to queueManager to reschedule

65e3254

Fix test case error

2aa5d26

Add exponential delay time retry

7715ad5

ningyougang force-pushed the complete-old-activation-when-update-action branch from 3185af0 to 7715ad5 Compare May 5, 2022 08:13

ningyougang changed the title ~~Complete old activation when update action~~ send old version memoryQueue's stale activation to queueManager when update action May 5, 2022

ningyougang added 2 commits May 6, 2022 10:23

Fix test case

b347f41

Make queueManager test case more stable

acdce96

jiangpengcheng reviewed May 7, 2022

View reviewed changes

ningyougang added 2 commits May 7, 2022 16:51

Optimize memoryQueue test case

eed0119

Make test case more stable

691497c

ningyougang commented May 7, 2022

View reviewed changes

style95 reviewed May 9, 2022

View reviewed changes

Change method name more readable

f7f8fff

style95 approved these changes May 9, 2022

View reviewed changes

ningyougang merged commit 37f1f99 into apache:master May 9, 2022

	private def handleStaleActivationsWhenUpdateAction(queueManager: ActorRef): Unit = {
	private def handleStaleActivationsWhenActionUpdated(queueManager: ActorRef): Unit = {

send old version memoryQueue's stale activation to queueManager when update action #5228

send old version memoryQueue's stale activation to queueManager when update action #5228

Uh oh!

Conversation

ningyougang commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang May 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang commented May 9, 2022

Uh oh!

Uh oh!

ningyougang commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading

ningyougang May 5, 2022 •

edited

Loading

ningyougang May 7, 2022 •

edited

Loading